Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some optimizations #199

Closed
wants to merge 4 commits into from
Closed

Some optimizations #199

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 29, 2013

Replaced function calls to array_push with adding the elements directly. This has 2 important advantages.

  1. Speed
  2. The code looks better this way.

@magento-team
Copy link
Contributor

Have you tried benchmarking this change in real environment eg homepage load time?

@@ -217,7 +217,7 @@ public function getOutput($clearPrevious = true)
*/
public function pushSilent()
{
array_push($this->_silentSaved, $this->_silent);
$this->_silentSaved = $this->_silent;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the missing [] on purpose?

@ghost
Copy link
Author

ghost commented Feb 5, 2013

mage2-team not in this case. I did it in a single loop. However, all evidence found on google points to the fact that array_push is a lot slower and it's quite obvious, since it's a function reference.

@airbone42
Copy link

I can confirm that from a lot of other projects

@hectorj
Copy link

hectorj commented Feb 6, 2013

So should we replace all uses of function aliases as well ?
The PHP documentation says "It is usually a bad idea to use these kind of aliases, as they may be bound to obsolescence or renaming, which will lead to unportable script." ( http://php.net/manual/en/aliases.php )

Edit : what tools would you suggest to benchmark a Magento ?

@qrz-io
Copy link
Contributor

qrz-io commented Feb 20, 2013

From the array_push documentation:

Note: If you use array_push() to add one element to the array it's better to use $array[] = because in that way there is no overhead of calling a function.

Even if the overhead is minimal, the improvement should not be overlooked, as it is an easy fix. Also, easier to read and the rest applies.

magento-team added a commit that referenced this pull request Nov 22, 2013
* Moved general action-related functionality to \Magento\App\Action\Action in the library. Removed Magento\Core\Controller\Varien\Action and related logic from the Magento_Core module
* Moved view-related methods from action interface to \Magento\App\ViewInterface with corresponding implementation
* Moved redirect creation logic from the action interface to \Magento\App\Response\RedirectInterface
* Moved Magento\Core common blocks to the library
* Added reading of etc/integration/config.xml and etc/integration/api.xml files for API Integrations
* Various improvements:
  * Email-related logic from the Core and Adminhtml modules consolidated in the new Email module
* GitHub requests:
  * [#238](#238) -- Improve escaping HTML entities in URL
  * [#199](#199) -- Replaced function calls to array_push with adding the elements directly
  * [#182](#182) -- By default use collection _idFieldName for toOption* methods.
  * [#233](#233) -- Google Rich Snippet Code
  * [#339](#339) -- Correcting 'cahce' typo in documentation.
  * [#232](#232) -- Update app/code/core/Mage/Checkout/controllers/CartController.php (fix issue #27632)
* Fixed bugs:
  * Fixed JavaScript error when printing orders from the frontend
  * Fixed Captcha problems on various forms when Captcha is enabled on the frontend
  * Fixed "Page not found" on category page if setting "Add Store Code to Urls" to "Yes" in the backend config
  * Fixed Fatal error when creating shipping label for returns
@verklov
Copy link
Contributor

verklov commented Nov 25, 2013

Hello and-ers,
We have processed your pull request. The code should be available in version dev53 released last Friday.

Thank you for your contribution!

@verklov verklov closed this Nov 25, 2013
magento-team pushed a commit that referenced this pull request Apr 3, 2015
[Firedrakes] MTF Configuration code clean up
MomotenkoNatalia pushed a commit that referenced this pull request Nov 24, 2015
VitaliyBoyko pushed a commit to VitaliyBoyko/magento2 that referenced this pull request Nov 25, 2017
…bsite_to_default_stock

magento#199 - Assign main website to default stock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants